[Federation] Add an end-to-end test verifying that deleting a federated namespace deletes child replicasets. #41278

perotinus · 2017-02-11T00:51:00Z

Verifies #38225.

Also, remove a few custom package aliases.

k8s-ci-robot · 2017-02-11T00:51:01Z

Hi @perotinus. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2017-02-11T00:51:02Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2017-02-11T00:51:03Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-reviewable · 2017-02-11T00:51:07Z

This change is

perotinus · 2017-02-11T00:55:58Z

cc @madhusudancs

perotinus · 2017-02-11T01:14:28Z

I signed it!

madhusudancs · 2017-02-11T04:17:26Z

A couple of minor nits but mostly looks good.

Also, please rebase.

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

test/e2e_federation/federated-namespace.go, line 103 at r1 (raw file):

			nsName := createNamespace(f.FederationClientset.Core().Namespaces())
			rsName := v1.SimpleNameGenerator.GenerateName(eventNamePrefix)

By convention, eventNamePrefix is used for Event API type object names. Please define another const for replicasets and use that const here.

test/e2e_federation/federated-namespace.go, line 143 at r1 (raw file):

			By(fmt.Sprintf("Verify that replicaset %s was deleted as well", rsName))
			err = wait.Poll(5*time.Second, wait.ForeverTestTimeout, func() (bool, error) {

I recommend pulling this and

kubernetes/test/e2e_federation/federated-replicaset.go

Line 417 in a50ecde

err = wait.Poll(5*time.Second, wait.ForeverTestTimeout, func() (bool, error) {

into a common helper function.

Comments from Reviewable

perotinus · 2017-02-13T22:15:45Z

Thanks, @madhusudancs! PTAL when you get a chance.

perotinus · 2017-02-13T22:16:37Z

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

test/e2e_federation/federated-namespace.go, line 103 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

By convention, eventNamePrefix is used for Event API type object names. Please define another const for replicasets and use that const here.

Done.

test/e2e_federation/federated-namespace.go, line 143 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

I recommend pulling this and

kubernetes/test/e2e_federation/federated-replicaset.go

Line 417 in a50ecde

err = wait.Poll(5*time.Second, wait.ForeverTestTimeout, func() (bool, error) {

into a common helper function.

Done.

Comments from Reviewable

madhusudancs · 2017-02-13T22:32:16Z

Reviewed 2 of 3 files at r2.
Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions.

test/e2e_federation/federation-util.go, line 530 at r2 (raw file):

}

/*

It is a convention to use // on each line for docstrings.

Comments from Reviewable

perotinus · 2017-02-13T22:34:48Z

Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions.

test/e2e_federation/federation-util.go, line 530 at r2 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

It is a convention to use // on each line for docstrings.

Ah, OK. Done.

Should we fix the other comments in this file, perhaps in a follow-up PR?

Comments from Reviewable

madhusudancs · 2017-02-13T22:38:47Z

Reviewed 1 of 3 files at r2, 1 of 2 files at r3.
Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

test/e2e_federation/federation-util.go, line 530 at r2 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

Ah, OK. Done.

Should we fix the other comments in this file, perhaps in a follow-up PR?

Follow up PR for other instances would be great. I will LGTM and approve this PR.

Comments from Reviewable

madhusudancs · 2017-02-13T22:39:15Z

/lgtm
/approve

Reviewed 1 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

…ed namespace deletes child replicasets. Verifies kubernetes#38225. Also, remove a few custom package aliases.

k8s-github-robot · 2017-02-13T22:47:12Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: madhusudancs, perotinus

Needs approval from an approver in each of these OWNERS Files:

~~test/e2e_federation/OWNERS~~ [madhusudancs]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

perotinus · 2017-02-13T22:49:25Z

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

test/e2e_federation/federation-util.go, line 530 at r2 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Follow up PR for other instances would be great. I will LGTM and approve this PR.

Thanks! I'll start working on that now.

Comments from Reviewable

perotinus · 2017-02-13T23:07:16Z

@k8s-bot verify test this

k8s-ci-robot · 2017-02-13T23:07:18Z

@perotinus: you can't request testing unless you are a kubernetes member.

In response to this comment:

@k8s-bot verify test this

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

madhusudancs · 2017-02-13T23:10:38Z

@k8s-bot ok to test

perotinus · 2017-02-14T01:19:56Z

@madhusudancs Can you re-LGTM?

madhusudancs · 2017-02-14T01:47:31Z

/lgtm

k8s-github-robot · 2017-02-14T05:07:11Z

Automatic merge from submit-queue (batch tested with PRs 41357, 41178, 41280, 41184, 41278)

…namespace deletion. See kubernetes#41278 for the original test.

Automatic merge from submit-queue (batch tested with PRs 41505, 41484, 41544, 41514, 41022) [Federation] Fixes the newly-added test for replicaset deletion upon namespace deletion. See #41278 for the original test. **Release note**: ```release-note NONE ```

…namespace deletion. See kubernetes#41278 for the original test.

nikhiljindal · 2017-03-06T21:46:58Z

test/e2e_federation/federated-namespace.go

+
+			By(fmt.Sprintf("Verify that replicaset %s was deleted as well", rsName))
+
+			waitForReplicaSetToBeDeletedOrFail(f.FederationClientset, nsName, rsName)


We should not require a wait here. Replicaset should have been deleted before the namespace was deleted.

You mean the delete namespace operation is synchronous, i.e. it won't return until the namespace cleanup is complete?

the function that this test is using to delete namespace is synchronous. It waits for the namespace to be deleted before returning.
So here, we can directly check that the replicaset is gone. No need to wait again.

After following this commentary, I'd suggest appending a suffix to deleteNamespace to indicate that it is synchronous (+Sync or +AndWait, whichever....).

@nikhiljindal makes sense.

@csbell good idea.

@perotinus is this something that you can fix?

We use AndWait in kubernetes tests often, so we can stick with that I guess.

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 11, 2017

k8s-github-robot assigned csbell Feb 11, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Feb 11, 2017

madhusudancs added the sig/federation label Feb 11, 2017

perotinus force-pushed the nsdeletion-e2etest branch 3 times, most recently from caf0f25 to 1bd1408 Compare February 11, 2017 01:12

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 11, 2017

madhusudancs added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Feb 11, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2017

perotinus force-pushed the nsdeletion-e2etest branch 2 times, most recently from c7d231d to 089c589 Compare February 13, 2017 22:14

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2017

perotinus force-pushed the nsdeletion-e2etest branch from 089c589 to 0eba31d Compare February 13, 2017 22:34

madhusudancs self-assigned this Feb 13, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2017

[Federation] Add an end-to-end test verifying that deleting a federat…

2e7683d

…ed namespace deletes child replicasets. Verifies kubernetes#38225. Also, remove a few custom package aliases.

perotinus force-pushed the nsdeletion-e2etest branch from 0eba31d to 2e7683d Compare February 13, 2017 22:46

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2017

k8s-github-robot merged commit 8db5ca1 into kubernetes:master Feb 14, 2017

perotinus added a commit to perotinus/kubernetes that referenced this pull request Feb 15, 2017

[Federation] Fixes the newly-added test for replicaset deletion upon …

80205b2

…namespace deletion. See kubernetes#41278 for the original test.

perotinus mentioned this pull request Feb 15, 2017

[Federation] Fixes the newly-added test for replicaset deletion upon namespace deletion. #41505

Merged

ahakanbaba pushed a commit to ahakanbaba/kubernetes that referenced this pull request Feb 17, 2017

[Federation] Fixes the newly-added test for replicaset deletion upon …

49ceea4

…namespace deletion. See kubernetes#41278 for the original test.

nikhiljindal reviewed Mar 6, 2017

View reviewed changes

nikhiljindal mentioned this pull request Mar 6, 2017

federation: deleting ns does not delete rs #38225

Closed

perotinus deleted the nsdeletion-e2etest branch May 10, 2017 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Federation] Add an end-to-end test verifying that deleting a federated namespace deletes child replicasets. #41278

[Federation] Add an end-to-end test verifying that deleting a federated namespace deletes child replicasets. #41278

perotinus commented Feb 11, 2017

k8s-ci-robot commented Feb 11, 2017

k8s-ci-robot commented Feb 11, 2017

k8s-ci-robot commented Feb 11, 2017

k8s-reviewable commented Feb 11, 2017

perotinus commented Feb 11, 2017

perotinus commented Feb 11, 2017

madhusudancs commented Feb 11, 2017

perotinus commented Feb 13, 2017

perotinus commented Feb 13, 2017

madhusudancs commented Feb 13, 2017

perotinus commented Feb 13, 2017

madhusudancs commented Feb 13, 2017

madhusudancs commented Feb 13, 2017

k8s-github-robot commented Feb 13, 2017

perotinus commented Feb 13, 2017

perotinus commented Feb 13, 2017

k8s-ci-robot commented Feb 13, 2017

madhusudancs commented Feb 13, 2017

perotinus commented Feb 14, 2017

madhusudancs commented Feb 14, 2017

k8s-github-robot commented Feb 14, 2017

nikhiljindal Mar 6, 2017

madhusudancs Mar 6, 2017

nikhiljindal Mar 6, 2017

csbell Mar 6, 2017

madhusudancs Mar 7, 2017

madhusudancs Mar 7, 2017


		By(fmt.Sprintf("Verify that replicaset %s was deleted as well", rsName))

		waitForReplicaSetToBeDeletedOrFail(f.FederationClientset, nsName, rsName)

[Federation] Add an end-to-end test verifying that deleting a federated namespace deletes child replicasets. #41278

[Federation] Add an end-to-end test verifying that deleting a federated namespace deletes child replicasets. #41278

Conversation

perotinus commented Feb 11, 2017

k8s-ci-robot commented Feb 11, 2017

k8s-ci-robot commented Feb 11, 2017

k8s-ci-robot commented Feb 11, 2017

k8s-reviewable commented Feb 11, 2017

perotinus commented Feb 11, 2017

perotinus commented Feb 11, 2017

madhusudancs commented Feb 11, 2017

perotinus commented Feb 13, 2017

perotinus commented Feb 13, 2017

madhusudancs commented Feb 13, 2017

perotinus commented Feb 13, 2017

madhusudancs commented Feb 13, 2017

madhusudancs commented Feb 13, 2017

k8s-github-robot commented Feb 13, 2017

perotinus commented Feb 13, 2017

perotinus commented Feb 13, 2017

k8s-ci-robot commented Feb 13, 2017

madhusudancs commented Feb 13, 2017

perotinus commented Feb 14, 2017

madhusudancs commented Feb 14, 2017

k8s-github-robot commented Feb 14, 2017

nikhiljindal Mar 6, 2017

Choose a reason for hiding this comment

madhusudancs Mar 6, 2017

Choose a reason for hiding this comment

nikhiljindal Mar 6, 2017

Choose a reason for hiding this comment

csbell Mar 6, 2017

Choose a reason for hiding this comment

madhusudancs Mar 7, 2017

Choose a reason for hiding this comment

madhusudancs Mar 7, 2017

Choose a reason for hiding this comment